Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Post Authentication Listeners #22275

Closed

Conversation

kleijnweb
Copy link

Q A
Branch? master
Bug fix? no
New feature? yes/no
BC breaks? no
Deprecations? no
Tests pass? yes/no
Fixed tickets #22260 (partly)
License MIT
Doc PR Pending approval

This PR is a first step towards #22260 by enabling the use of anon and post_authentication as listener sorting positions, opening up extending the firewall beyond authentication. To that end, returning an authentication provider ID from SecurityFactoryInterface::create() has been made optional.

A typical use case would be creating custom authorization listeners (alternatives to using access_control while still benefiting from e.g. the exception listener).

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 4, 2017
@kleijnweb
Copy link
Author

Waiting for master to be fixed before I push deprecation fix :)

Copy link
Contributor

@linaori linaori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the issue, the anon. related changes, I'm 👍. For the post_authentication I still don't see a valid use-case so I'm 👎 for that.

My suggestion is to keep the anon. part in this PR and make a new one based on this PR for the post_authentication. This because the anon. part can be kept regardless of what will be decided upon post_authentication.

@kleijnweb
Copy link
Author

@iltar

I am inclined to agree, but after more refactoring efforts elsewhere I ran into a possible issue with the anon sorting position:

Enabling anon as possible position means that you can use a custom AnonymousListener. Nothing wrong with that, IMHO, but the literal key anonymous (which is now reserved for the bundled listener) is used to search the array of listeners keys in FirewallConfig to determine allowsAnonymous().

Which begs the question: should the position of a listener as returned by a factory be interpret as the type of a listener? I don't think this is the case for pre_auth, especially not in the way guard uses it.

So maybe a sorting position with a different semantic value would make more sense.

@fabpot
Copy link
Member

fabpot commented Jul 6, 2017

What's the status of this PR?

@kleijnweb
Copy link
Author

@fabpot

  1. There doesn't seem to be any support for adding a "post_authentication" listening position. @iltar seems to think I am trying to solve a problem that doesn't exist and by doing so am inviting improper use of firewall listeners (which I don't agree with, but there doesn't seem to be much point in further debating the issue)

  2. There was support for the refactoring I did (extracting the anonymous listener), but in doing so I discovered that the semantic value of "listener positions" is confused: being used as "type" in FirewallConfig::allowsAnonymous() and perhaps elsewhere. This reduces the value of the refactoring to a point where, considering the general lack of interest in refactoring the security component, it is not worth the trouble.

Unless any of this has changed, this should be closed (#22260 too).

@fabpot
Copy link
Member

fabpot commented Jul 6, 2017

Thank you @kleijnweb for the quick and honest feedback. I really appreciate it. I'm going to trust @iltar judgement on this one and close.

@fabpot fabpot closed this Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants